Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cross platform input problems #60

Merged
merged 1 commit into from
Jun 10, 2016
Merged

Conversation

runarberg
Copy link
Contributor

  • Fifth level keys were not working. That prevented some inputting
    important characters (like pipe | and caret ^) on some keyboards.
  • Control characters didn't prevent default browser behavior on some
    platforms. ctrl + D for example sent the EOT and opened the
    bookmark section on MS Edge.
  • Meta key behavior simply wasn't working on Mac OS. There is some logic
    that maps metaKey on Mac OS to the altKey on other
    systems. However that logic didn't work.

Instead of parsing the user-agent string to find the users os, we now
look into the platform attribute of the navigator object.

@runarberg
Copy link
Contributor Author

I've been having some serious cross platform inconsistencies using this package. Most serious is not being able to input fifth-level keys with my Icelandic keyboard. Meaning I'm unable to enter keys like the pipe |, tilde ~ or backtick ```. Also on some my mac the meta key isn't working (I have to press esc + `d` for "forward delete word" for example, while `alt + d` works on my linux). And on top of that some control sequences trigger browser side-effects (like `ctrl + k`, "kill to end of line", opens a new tab with current URL on Edge on my Windows machine).

I've tested this with Icelandic keyboard on firefox 46.0.1 and chrome 50.0.2661.102 (64-bit) on Mac OS X 10.11.5 as well as on MS Edge on Windows 10.

Note that this brakes copy-pasting using ctrl + C and ctrl + V on windows and cmd + C and cmd + V on Mac OS X. You can still copy by right click → copy on all browsers, and paste by editpaste in chrome and (sometimes) in Edge. This can (maybe) be fixed by creating a custom context menu on terminal that includes a paste button, that will paste from the clipboard into the textbox.

@parisk
Copy link
Contributor

parisk commented Jun 2, 2016

Hi @runarberg, thanks a lot for your contribution. Let me make a few comments please:

Instead of parsing the user-agent string to find the users os, we now look into the platform attribute of the navigator object.

This is a great addition. It actually sounds a lot more sane utilizing the user agent's platform attribute.

As far as hitting different combinations of Ctrl + any key that trigger default browser behavior, this should not be handled by xterm.js, but by the application that uses it. Since xterm.js is just a front-end terminal emulator, it does not imply its use case. For example:

  • it could be used in an Electron app (desktop app in html/css/js), where the Ctrl + K does something that should not be prevented by xterm.js (e.g. Ctrl + K could be displaying keyboard shortcuts there)
  • it could be used to make front-end terminal-like components, where Ctrl + any key does not make sense.

Last, copy and paste should not be broken for any platforms, as xterm.js is only a front-end terminal emulator. While Ctrl + C might make sense not to copy in Windows, but send a keyboard interrupt to a back-end Linux terminal, this case should not be handled by xterm.js itself, but by the parent application.

Also would it be able to apply particular rules based on the language of the user agent (use navigator.language or navigator.languages?

Do you believe we could fix these comments, in order to merge this PR?

Feel free to open also an issue, in order to discuss anything you believe we need to.

// TODO: This also brakes copy-pasting into the terminal using
// ``ctrl + C'' and ``ctrl + V''. Find a different way of
// allowing users to copy-paste intuitively.
this.cancel(ev, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parisk If I remove this part, would you consider merging the rest?

@runarberg
Copy link
Contributor Author

@parisk I don't know if the language and languages attributes will help for a few reason, at least not for detecting the input.

  1. There is not a clear mapping between language and keyboards. Some languages have several popular layouts, for example, some Esperanto layouts use dead keys to enter esperanto specific characters, while others override the non-esperanto characters (yŭ) and then use the fifth level to reach the english equivalent.
  2. A user might have a preferred language, but still prefer a keyboard language in another language. Because the task at hand is non-optimal using their preferred language’s keyboard layout.
  3. A user might have a custom layout that deviates from their preferred language, for example a default keyboard layout for a particular language might use dead keys to enter symbol, but a user might have remapped them to use a fifth level key.
  4. A user might simply fail to set a preferred language in their system, and therefor the attributes will give a false image of the user.

In this PR I opted for detecting the system, rather than the preferred language, because generally the popular OS platforms are consistent in how a fifth-level key is reached:

  • Windows machines use Ctrl + Alt + key, also mapped to AltGr.
  • Mac OS machines use ⌥ Option + key.
  • Linux machines (X windowing system) have a native notion of ISO_Level5_Shift that is usually mapped to the AltGr on modern keyboards (although for us it doesn't matter since the DOM event handles it correctly).

@runarberg
Copy link
Contributor Author

I amended the commit. I stopped preventing the browsers default behavior, stopped referring to the “fifth-level” (after some investigation, it is actually called a “third level shift”), and simplified the commit message.

@parisk
Copy link
Contributor

parisk commented Jun 6, 2016

Thanks for the info and commits @runarberg. What you are writing sound quite rational. We just have to make sure that they keep the desired user experience for most of potential users.

I will give a deeper look into this today and also make a local rebase with #78 to see if everything works 🆗 .

@parisk
Copy link
Contributor

parisk commented Jun 7, 2016

I pulled this branch, checked it out, rebased with master and still I cannot copy and paste using my keyboard on OS X.

The existing experience should not break for the majority of users. @runarberg can you please list all behavior that is expected to break after your latest patches and possible workarounds for each of them?

@runarberg
Copy link
Contributor Author

@parisk This is because we are using ⌘ Cmd on Mac OS as Alt on other platforms. If you prevent default behavior on the terminal key event, this key won't emit default behavior. For example ⌘ Cmd + C will act as Alt + C on other systems, and emit “Capitalize word from cursor” to the terminal instead of “Copy” to the browser window.

This is the only expected behavior that brakes that I can think of, unless some windows users have browser default behavior to Ctrl + Alt + key.

@runarberg
Copy link
Contributor Author

runarberg commented Jun 8, 2016

I amended the commit, stopped using Mac OS ⌘cmd as Alt on other systems (now Mac OS users have to use Esc like before), and rebased onto upstream/master. Keyboard copy-pasting works again on Macs.

Ps. I discovered an unintended side-effect where chrome users on ubuntu can use ⇧shift + Ctrl + V to paste. ⇧shift + Ctrl + C wont work for copying, nor does this work on firefox, nor edge on windows.

@parisk
Copy link
Contributor

parisk commented Jun 8, 2016

Thanks a lot for making progress on this. I move forward with pulling your branch and testing right now.

@parisk
Copy link
Contributor

parisk commented Jun 8, 2016

Okay the only issue that I found out is that Ctrl + Shift + V. Won't work for pasting in Firefox. Was that just me or happens to you as well?

Also I think that we can manage with making Ctrl + Shift + C work, as it invokes the developer tools by default.

@runarberg
Copy link
Contributor Author

@parisk I don't think Ctrl + Shift + key should work as a replacement for Ctrl + key in firefox on ubuntu, it didn't doesn't do so upstream either. It works as a replacement in chrome (and Terminator) for some reason.

We could bind Ctrl + Shift + {V, C} to document.execCommand("paste") and document.execCommand("copy") (again paste won't work in firefox unless the user changes their settings), but I don't believe that belongs in this PR. Ubuntu and Windows users could also use Shift + Insert to paste, but xterm.js will send a \x1b[2~ to the terminal (I can send another PR that fixes that, but it doesn't really belong here since it is also the behavior upstream).

@runarberg
Copy link
Contributor Author

I submitted #107 to fix keyboard copy-pasting on non-macs.

@runarberg runarberg force-pushed the master branch 2 times, most recently from b9350b8 to 3d00632 Compare June 9, 2016 14:44
@runarberg
Copy link
Contributor Author

Ammended: Fixed a bug in the last patch, and rebased onto upstream.

@parisk
Copy link
Contributor

parisk commented Jun 10, 2016

Hi @runarberg, thanks again for your patches. It seems that Alt + / does not work any more to move back and forward with words (tested on Chrome on Mac).

Is there any workaround for this?

@parisk
Copy link
Contributor

parisk commented Jun 10, 2016

Also could you please provide us some documentation/reference in order to see how third level keys should work?

This way we will be able to test the new functionality on top of the existing one and make sure things won't break for mainstream users.

ISO third level keys were not working. That prevented some inputting
important characters (like pipe `|` and caret `^`) on some keyboard
layouts.

Instead of parsing the user-agent string to find the users os, we now
look into the `platform` attribute of the `navigator` object.

Removed the hijacking of the command key `⌘` on Mac OS as the `Alt` key
on other systems.
@runarberg
Copy link
Contributor Author

runarberg commented Jun 10, 2016

Amended: Fixed Alt + /, added unit tests, and rebased on top of upstream.

@runarberg
Copy link
Contributor Author

@parisk I don't really know much about keyboard input methods, except from experience of using ISO keyboard, and frustration when ANSI keyboard is forced upon me. But here is a wikipedia article about the ISO keyboard standard.

@parisk
Copy link
Contributor

parisk commented Jun 10, 2016

👍 thanks a lot for one more time. This seems to be working great now. Merging.

@parisk parisk merged commit 7c1559b into xtermjs:master Jun 10, 2016
bfcapell added a commit to hackermirror/term.js that referenced this pull request May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants